feat: Return local nonce when getTransactionCount request is signed #151
Conversation
| @@ -58,7 +60,7 @@ func NewRpcRequest( | |||
| rpcCache *application.RpcCache, | |||
| ) *RpcRequest { | |||
| return &RpcRequest{ | |||
| logger: logger, | |||
| logger: logger.With("method", jsonReq.Method), | |||
adapters/flashbots/signature.go
Outdated
| return VerifySignature(body, splitSig[0], splitSig[1]) | ||
| } | ||
|
|
||
| func VerifySignature(body []byte, signingAddressStr, signatureStr string) (string, error) { |
There was a problem hiding this comment.
explicit names for the return values would increase clarity
| func VerifySignature(body []byte, signingAddressStr, signatureStr string) (string, error) { | |
| func VerifySignature(body []byte, signingAddressStr, signatureStr string) (signaturePubkeyHex string, err error) { |
There was a problem hiding this comment.
Add in d90fb9b, still need to write unit tests those are coming next.
| @@ -134,11 +134,11 @@ func (r *RpcRequestHandler) process() { | |||
| r.logger = r.logger.New("rpc_method", jsonReq.Method) | |||
|
|
|||
| // Process single request | |||
| r.processRequest(client, jsonReq, origin, referer, isWhitehatBundleCollection, whitehatBundleId, urlParams, r.req.URL.String()) | |||
There was a problem hiding this comment.
I wonder if a better approach would be not to pass the full body around for possible later signature check, but instead here do a signature check if the header is present, and only pass the result around.
Downside of that approach: doing the signature check also on requests that wouldn't need it. Upside: everything is a bit simpler.
There was a problem hiding this comment.
Ya I like that too, I'll make that change and add tests for the signatures.
There was a problem hiding this comment.
Refactored to that approach in d90fb9b.
| @@ -150,3 +152,48 @@ func (r *RpcRequest) intercept_eth_call_to_FlashRPC_Contract() (requestFinished | |||
| r.logger.Info("Intercepted eth_call to FlashRPC contract") | |||
| return true | |||
| } | |||
|
|
|||
| func (r *RpcRequest) intercept_signed_eth_getTransactionCount() (requestFinished bool) { | |||
There was a problem hiding this comment.
I would love if we could start doing unit / e2e tests for these handlers, but that seems def out of scope for now.
|
LGTM! Left a few minor comments. Most notably, please add tests to the signature methods. I don't love the name |
server/request_intercepts.go
Outdated
| if errors.Is(err, flashbots.ErrNoSignature) { | ||
| r.logger.Info("[eth_getTransactionCount] No signature found") | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
Oh wow just realized when refactoring this isn't handling the else case and should be returning an error result. The refactor I'm doing to move the signature check earlier in the flow helps make that more obvious 😊
There was a problem hiding this comment.
Fixed in d90fb9b.
…er to avoid storing request body.
📝 Summary
Implements an "MVP" version of #150: When a
eth_getTransactionCountRPC request is signed with aX-Flashbots-Signatureheader matching the target address we return the "next nonce" from Redis instead of proxying the request.⛱ Motivation and Context
This feature is desirable for Wallets that want to use
eth_getTransctionCountto manage user nonces while still preserving users' privacy.Note that this is the MVP implementation on this functionality, in that it uses the existing
RState.GetSenderMaxNonce(addr)check. As noted in #150 this current code does not respect "nonce gaps" so a follow-on PR will be needed later to correctly handle nonce gaps (most likely via adding a per-address Redis Sorted Set ("ZSet") as mentioned in the issue). However, this implementation will work for most scenarios, and will allow us to get feedback from wallets before working on the more-detailed implementation.📚 References
Tested as follows:
✅ I have run these commands
make lintmake testgo mod tidy